feat: chat UI#1112
feat: chat UI#1112leonardmq wants to merge 6 commits intoleonard/kil-447-feat-stream-multiturn-ai-sdk-openai-protocolsfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Coverage ReportOverall Coverage: 90% Diff: origin/leonard/kil-447-feat-stream-multiturn-ai-sdk-openai-protocols...HEAD
Summary
Line-by-lineView line-by-line diff coverageapp/desktop/studio_server/chat_api.pyLines 28-46 Lines 52-61 Lines 79-88 Lines 120-129 Lines 133-147 libs/core/kiln_ai/adapters/chat/chat_formatter.pyLines 274-282 libs/core/kiln_ai/adapters/model_adapters/base_adapter.pyLines 692-700 Lines 732-742 Lines 742-753 libs/core/kiln_ai/adapters/model_adapters/litellm_adapter.pyLines 698-707 libs/core/kiln_ai/datamodel/tool_id.pyLines 85-93 libs/core/kiln_ai/tools/client_tool.pyLines 53-61
|
|
Bug:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new, fully functional chat interface within the application. The core motivation for this feature was to provide an interactive AI chat experience, which necessitated a custom streaming implementation due to existing framework version constraints. The new UI integrates robust markdown rendering with security features, ensuring a rich and safe display of AI-generated content. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new chat UI, including dependencies, a markdown rendering component, client-side logic for streaming chat, and the chat page itself. A critical security concern has been identified: the DOMPurify configuration is overly permissive, allowing attributes like class and target which can be exploited for UI redressing and phishing attacks. Additionally, raw error messages from the backend API are exposed, potentially leading to information leakage. Beyond security, there are also opportunities to improve code maintainability by reducing redundancy.
| "span", | ||
| "div", | ||
| ] | ||
| const ALLOWED_ATTR = ["href", "target", "rel", "class"] |
There was a problem hiding this comment.
The DOMPurify configuration allows the class attribute on all allowed tags. When using a utility-first CSS framework like Tailwind CSS, this allows an attacker to inject arbitrary utility classes into the rendered markdown. An attacker can use this to perform UI redressing, such as overlaying fake buttons, hiding parts of the UI, or mimicking system notifications, which can lead to phishing or other social engineering attacks.
| "span", | ||
| "div", | ||
| ] | ||
| const ALLOWED_ATTR = ["href", "target", "rel", "class"] |
There was a problem hiding this comment.
The DOMPurify configuration allows the target attribute on <a> tags. An attacker can use this to set target="_self" or target="_top", allowing them to navigate the current tab or the entire window to a malicious site when the user clicks a link in the chat. This can be used for phishing attacks where the attacker mimics the application's UI on their own site.
| const text = await response.text() | ||
| onError( | ||
| new Error( | ||
| `Chat API error ${response.status}: ${text || response.statusText}`, |
There was a problem hiding this comment.
The streamChat function captures the raw response body from the API when a request fails and includes it in the Error object passed to the onError callback. This error message is then displayed directly in the UI in +page.svelte. If the backend API returns sensitive information (e.g., system paths, stack traces, or internal configuration) in the error response, it will be exposed to the user.
| hljs.registerLanguage("json", json) | ||
| hljs.registerLanguage("javascript", javascript) | ||
| hljs.registerLanguage("js", javascript) | ||
| hljs.registerLanguage("typescript", typescript) | ||
| hljs.registerLanguage("ts", typescript) | ||
| hljs.registerLanguage("python", python) | ||
| hljs.registerLanguage("py", python) | ||
| hljs.registerLanguage("bash", bash) | ||
| hljs.registerLanguage("shell", bash) | ||
| hljs.registerLanguage("sh", bash) |
There was a problem hiding this comment.
The language packs for highlight.js often include common aliases. For example, the javascript language pack includes the js alias, typescript includes ts, python includes py, and bash includes sh. Registering these aliases explicitly is redundant. You can simplify this section by removing the redundant registerLanguage calls.
hljs.registerLanguage("json", json)
hljs.registerLanguage("javascript", javascript)
hljs.registerLanguage("typescript", typescript)
hljs.registerLanguage("python", python)
hljs.registerLanguage("bash", bash)
hljs.registerLanguage("shell", bash)
| if (currentTextId === null) { | ||
| const id = nextSlotId() | ||
| partOrder.push({ kind: "text", id }) | ||
| currentTextId = id | ||
| textBlocks.set(id, "") | ||
| } |
There was a problem hiding this comment.
The logic to create a new text block when currentTextId is null is duplicated in multiple places (here, and in the text-delta handler). A similar duplication exists for reasoning blocks. This makes the code harder to maintain.
Consider refactoring this logic into a helper function. For example, a function like ensureCurrentTextBlock() could encapsulate this block creation logic and be called wherever needed. This would reduce code duplication and make the stream processing logic easier to follow.
2bfe312 to
cdd99bd
Compare
d411063 to
0bbab04
Compare
What does this PR do?
Vercel AI SDK has libraries for Svelte, but we use Svelte 4, and those libraries require >= Svelte 5. An ancient version of the library exists for Svelte 4, but it is tied to an older version of the protocol so would require our backend / Kiln SDK to also be on an old protocol version.
Changes:
Checklists